check for pseudo header in trailer by nateprewitt · Pull Request #305 · python-hyper/h2

nateprewitt · 2016-08-31T18:05:24Z

So, I'm quickly realizing that I'm way out of my element here with HTTP2, so please excuse my naivety.

From the quick reading I did, I think this workflow should fail with a ProtocolError, but currently doesn't. I'm not sure how state works with the connection object here, so it's possible I'm not using the frames correctly.

@Lukasa or @alexwlchan, could I get some confirmation that this is along the right path?

Lukasa · 2016-08-31T21:24:46Z

Hrm, right now I can't see what is going wrong because I'm flying and have no access to a proper shell (I'm on a phone in an airport), and Travis is failing the build on the linter. I'll follow this up tomorrow.

nateprewitt · 2016-08-31T21:45:45Z

Sorry, I was expecting the test to fail so I didn't check the reason. It's now failing by NOT raising the ProtocolError. I don't know if this is because the conditions I've created are insufficient to reach that code, or because the functionality isn't implemented yet. No rush here, have a good flight!

alexwlchan · 2016-09-01T05:22:39Z

I don't know if this is because the conditions I've created are insufficient to reach that code, or because the functionality isn't implemented yet.

I had a proper look, and it’s because the functionality isn’t implemented yet. So you’ll need to add it – if you look in utilities.py, there are a series of functions that do header block validation. You’ll need to find an appropriate place to perform this new check.

Looking at the test already written: looks fine to me (but I’m far from an HTTP/2 expert), but you should also check that we send the appropriate response (with an HTTP error code) back down the connection. If you look at test_headers_event, you’ll see we do this check after ensuring we hit the ProtocolError.

Lukasa · 2016-09-01T12:59:57Z

+        c.clear_outbound_data_buffer()
+
+        head_flag = ["END_HEADERS"]
+        stream_flag = ["END_STREAM"]


There's no reason for us to hoist these out, they can go straight into the relevant frame factory calls.

nateprewitt · 2016-09-01T13:56:23Z

Thanks for clarifying that @alexwlchan!

@Lukasa, sorry about all the junk, I didn't intend to have this reviewed yet. I just wanted to clarify that the test was working as intended before extending.

I'll start working on getting that exception raised :)

nateprewitt · 2016-09-01T17:42:36Z

@Lukasa, it looks like this test in test_complex_logic.py may need some tweaks. It is currently sending two sets of headers, both with a :status, down a stream. When it sends the second set of headers to tear down the inbound stream, the frame is flagged as being a trailer which causes the validate_header check to fail. This doesn't fail for the what looks to be a very similar call on the outbound stream above. ~~I'm not sure I understand why one is failing while the other isn't, other than different checks depending on "direction" of the stream.~~

Some possible solutions are sending a different header set (sans pseudo-headers), or just a data frame. This may need some more investigating if I'm missing something deeper though.

edit: I realized that the outbound send is for a promise frame rather than a header frame. I'm not sure what the semantics of the promise frame are, but that's probably the reason why we're not getting an error on the outbound.

nateprewitt · 2016-09-01T18:00:11Z

Are there different rules from inbound v.s. outbound header frames? validate_outbound_headers seems to only do a subset of validate_headers.

Lukasa · 2016-09-01T18:01:33Z

A PUSH_PROMISE frame contains request headers, not response headers. Essentially, the server pushes a request and then sends its own response to it.

Lukasa · 2016-09-01T18:05:54Z

Are there different rules from inbound v.s. outbound header frames?

No, there aren't. However, some of the checks on inbound header frames don't apply to outbound headers because we normalize them first. There's no point checking for whitespace or uppercase header fields because we got rid of any violations already.

nateprewitt · 2016-09-01T18:12:32Z

Ok cool, thanks @Lukasa.

Is that second header frame supposed to be treated as a new response, or a trailer? I'll need to tweak it, or my validation check to get tests passing.

Lukasa · 2016-09-01T18:30:37Z

There is only one HEADERS frame. =) It is, therefore, not a trailer.

nateprewitt · 2016-09-01T18:35:59Z

Gah, sorry I'm being so sloppy with my links. This is the second HeaderFrame we're sending down the same stream. This is being flagged as a trailer presently because my link in the comment above is the first set of headers sent.

nateprewitt · 2016-09-01T22:20:25Z

        ]
-        assert headers == h2.utilities.validate_headers(
-            headers, hdr_validation_flags)
+        if not hdr_validation_flags.is_trailer:


This fixes the failure condition but raises a question of if we really want to use hdr_validation_combos with this test. This will never be a valid trailer header and we're now checking for trailers with pseudo headers elsewhere, so this seems a bit redundant.

If this is a special case that requires all the checks though, I like this over a blanket try/except ProtocolError catch since this should be the only condition we fail under.

I think you just want to add a new set of header validation combos that doesn't include the trailer set, and use that here.

nateprewitt · 2016-09-01T22:29:15Z

Ok, after some further reading, I'm fairly confident the build_headers_frame issue I poorly explained earlier is in fact an unintended frame sequence. I've updated the code to send a data frame instead. Tests are passing now, and I think this should cover #263.

Lukasa · 2016-09-02T05:25:14Z

-                headers=self.example_response_headers,
+                data=b'the content',
                flags=['END_STREAM'],
            )


Ok, seeing this change in place I now understand what you were getting at. Yes, there is nothing wrong with closing that stream using trailers, but the trailers that were being sent were not actually valid trailers. This change is better. =)

nateprewitt · 2016-09-02T06:10:17Z

Alright, I think things are tidied. Gonna call it a night but I'll finish off any final tweaks in the morning.

Lukasa · 2016-09-02T07:11:59Z

+    if hdr_validation_flags.is_trailer and seen_pseudo_header_fields:
+        raise ProtocolError(
+            "Received pseudo-header in trailer %s" %
+            seen_pseudo_header_fields.pop()


Eh, let's just show them all. We can just show the set directly. =)

nateprewitt · 2016-09-02T13:52:52Z

Voilà!

Lukasa · 2016-09-05T12:43:42Z

Alright, fantastic, thanks so much @nateprewitt! Per our one of the team policy, you've got an invite to the Python Hyper organisation in your inbox. Welcome on board!

nateprewitt force-pushed the 263_pseudo_header_in_trailer branch from 48f897e to 966fb96 Compare August 31, 2016 21:39

Lukasa reviewed Sep 1, 2016
View reviewed changes

nateprewitt force-pushed the 263_pseudo_header_in_trailer branch 4 times, most recently from fa28998 to 61106a2 Compare September 1, 2016 21:32

nateprewitt reviewed Sep 1, 2016
View reviewed changes

nateprewitt changed the title ~~[WIP] check for pseudo header in trailer~~ check for pseudo header in trailer Sep 1, 2016

nateprewitt force-pushed the 263_pseudo_header_in_trailer branch from b44d9ab to a09e16f Compare September 2, 2016 04:52

Lukasa reviewed Sep 2, 2016
View reviewed changes

nateprewitt force-pushed the 263_pseudo_header_in_trailer branch 2 times, most recently from 5069db1 to af26031 Compare September 2, 2016 06:05

Lukasa reviewed Sep 2, 2016
View reviewed changes

check for pseudo header in trailer

1327ed9

nateprewitt force-pushed the 263_pseudo_header_in_trailer branch from af26031 to 1327ed9 Compare September 2, 2016 13:47

Lukasa merged commit 1327ed9 into python-hyper:master Sep 5, 2016

Lukasa added a commit that referenced this pull request Sep 5, 2016

Changelog entry for #305.

a79b969

Lukasa mentioned this pull request Sep 5, 2016

Does hyper reject trailers that contain pseudo-header fields? #263

Closed

nateprewitt mentioned this pull request Sep 15, 2016

Reject response header blocks that don’t include the :status field #311

Merged

Conversation

nateprewitt commented Aug 31, 2016

Uh oh!

Lukasa commented Aug 31, 2016

Uh oh!

nateprewitt commented Aug 31, 2016

Uh oh!

alexwlchan commented Sep 1, 2016

Uh oh!

Lukasa Sep 1, 2016

Choose a reason for hiding this comment

Uh oh!

nateprewitt commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nateprewitt commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nateprewitt commented Sep 1, 2016

Uh oh!

Lukasa commented Sep 1, 2016

Uh oh!

Lukasa commented Sep 1, 2016

Uh oh!

nateprewitt commented Sep 1, 2016

Uh oh!

Lukasa commented Sep 1, 2016

Uh oh!

nateprewitt commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nateprewitt Sep 1, 2016

Choose a reason for hiding this comment

Uh oh!

Lukasa Sep 2, 2016

Choose a reason for hiding this comment

Uh oh!

nateprewitt commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lukasa Sep 2, 2016

Choose a reason for hiding this comment

Uh oh!

nateprewitt commented Sep 2, 2016

Uh oh!

Lukasa Sep 2, 2016

Choose a reason for hiding this comment

Uh oh!

nateprewitt commented Sep 2, 2016

Uh oh!

Lukasa commented Sep 5, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nateprewitt commented Sep 1, 2016 •

edited

Loading

nateprewitt commented Sep 1, 2016 •

edited

Loading

nateprewitt commented Sep 1, 2016 •

edited

Loading

nateprewitt commented Sep 1, 2016 •

edited

Loading